This topicName === '' expression appears here and also where we pass the disabled prop to a ZulipTextButton. Let's pull it out into a const validationErrors = … directly in the body of TopicEditModal, like we do in ComposeBox.

Then, let's make it a bit more sophisticated: topicName.trim() === '' (or equivalently, topicName.trim().length === 0). When the server rejects '' as a topic, it also rejects ' ', etc., so we should too, to avoid sending a request we know the server will reject.

Hmm, looking again at ComposeBox, I remember that sometimes servers do accept an empty topic: in particular, if the realm doesn't require topics in stream messages. So you could follow ComposeBox by adding a

const mandatoryTopics = useSelector(state => getRealm(state).mandatoryTopics);

and using mandatoryTopics in the new const validationErrors. So, specifically, if topicName.trim() === '' and mandatoryTopics, then say the input is invalid because topics are required and no topic was given.

You could also have validationErrors include an item for "the topic is too long," though I see you've handled that already with the maxLength prop on the input component, which seems OK. I slightly prefer doing that here for consistency, but could go either way.

I've implemented validationErrors. Please take a look at error message wording. 📝

nit: don't repeat the name in the jsdoc -- the name is already visible just below it

nit: this and some other formatting changes in commit 3, which otherwise doesn't touch this code which was added in commit 1. Instead, the commit that adds the code should give it the appropriate formatting in the first place.

Simplest way to fix this is probably to use git rebase -i to edit the first commit; then auto-format this file while there.

Generally we avoid having things float around with bogus values, like a "stream ID" that isn't actually the ID of any stream. That's because if we ever actually consult such a value and try to use it, that's likely to be a bug. So if we don't want to ever actually consult the value… best to write the data structures, and the types, so that we don't have the value lying around in the first place.

This is the error when I type the props to accept streamId: number | null:

Cannot call fetchSomeMessageIdForConversation with streamId bound to streamId because null [1] is incompatible with number [2].Flow(incompatible-call)

I didn't want to alter fetchSomeMessageIdForConversation. 😨

Indeed, we don't want to pass null as a stream ID to that function, and we should keep the types saying we don't do so.

We also don't want to pass it a bogus value like -1. So you should read this type error as telling you about a problem that's already there in the version with -1, and that the type-checker just isn't able to highlight for us in that version because using a bogus value -1 that has the same type as the legitimate values makes the issue invisible. Using null instead makes it visible to the type-checker where the lack of a real value can and can't flow, which is an improvement.

An adequate solution would be to say streamId: null | number (and similarly for the topic), and then add appropriate control flow so we don't attempt things like fetchSomeMessageIdForConversation when we don't actually have a stream and topic.

A further improvement would be to unify together the different states of visible, streamId, and oldTopic. It's always the case that when visible is true, we have an actual stream ID and topic, and when it's false, we don't. So ideally we'd express that in the state value's type, and then we need a smaller number of conditionals.

State unified!

Happy to learn about control flow. I'm not sure if I performed error handling correctly for the submit handler, but it now contains control flow that checks stream ID and topic for null.

nit: this comment doesn't seem to add anything not expressed in the case lines below; better to cut it

The handleSubmit in ComposeBox contains a similar comment. Would you like me to mark it for removal?

const handleSubmit = useCallback(() => {
//...
    if (validationErrors.length > 0) {
      const msg = validationErrors
        .map(error => {
          // 'upload-in-progress' | 'message-empty' | 'mandatory-topic-empty'
          switch (error) {
//...

Hmm, indeed! Just pushed a quick commit deleting that, and crediting you: bfc0fb6

The details of this style -- do they come from a particular style guide or design spec, or are they based on tweaking them until they looked about right?

I'm thinking in particular of:

• the backdrop as 25% black
• the modal's shadow
• the modal's width, padding, and corners
• the title's text color and other details

The backdrop as 25% black is from me trying to match the action sheet background when it's toggled.

The modal's shadow is from eyeballing, but I see now Popup.js has values I can borrow.

I've looked at the Material Design specs for dialogue and made some changes to the width and text color. The border radius is from CSS from ComposeBox.js.

Continuing from #5510 (comment) :

The backdrop as 25% black is from me trying to match the action sheet background when it's toggled.

Cool, that makes sense.

Let's have a one- or two-line comment here explaining that that's the reasoning for these choices of opacity and backgroundColor.

Similarly, let's have a comment saying that these are copied from Popup.js.

(Better yet might be to do some refactoring so that these details can live in one place and be shared. But that's fine to leave out of scope for this PR.)

nit: for this sort of error-handling control flow, put the handling right after the condition. Like this:

if (streamId !== null && oldTopic !== null && newTopicInputValue !== null) {
  throw new Error(_('Topic, streamId, or input value null.'));
}

Then the rest of the code can continue onward. Here's a previous discussion of a similar case:
#3888 (comment)

(I guess this is a reply to a question you asked above: #5510 (comment))

This conditional is effectively meant to ask "is the modal currently inactive/hidden". But it disagrees with some conditionals in TopicEditModal about exactly what that condition should be:

• this makes it inactive just if both streamId and oldTopic are null
• but those make it inactive just if either of those is null.

That means that if we somehow get into a state where just one of those is null, then these different parts of the code will behave inconsistently: the modal won't actually be shown (the visible prop on Modal will be false), but if the user tries to edit a topic nothing will happen (because this conditional will effectively believe the modal is still visible.)

I believe in this version of the code there's no way to enter that state. But there's nothing particularly to prevent that from becoming possible with some future change to the code. That makes it an example of a "latent bug".

One fix would be to make this an explicit invariant: add an assertion somewhere, either as an if and throw or with the handy invariant function:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#invariant-assertions

But there's actually a cleaner solution available. As a sort of followup to #5510 (comment) with eliminating bogus values and unifying visible with the other two pieces of this data, we can go another step further and adjust the type so that it excludes the should-be-impossible case of having one null property and one non-null. For the state type, we can replace this:

{
    oldTopic: null | string,
    streamId: null | number,
}

with this:

null | { oldTopic: string, streamId: number }

I think a number of spots in the code will get to be a bit cleaner that way.

Seeing this in the UI, it feels kind of wordy. It also can overflow the input:

Let's go for just "Topic". It is a placeholder, not an instruction, so it doesn't need to say a lot.

formatting nit: give each jsdoc a summary line, as its own paragraph. Like this:

   /**
    * True just if the button is in the disabled state.
-   *   https://material.io/design/interaction/states.html#disabled
+   *
+   * https://material.io/design/interaction/states.html#disabled
    */
   disabled?: boolean,
 
   /**
-   * Whether `onPress` is used even when
-   *   `disabled` is true.
+   * Whether `onPress` is used even when `disabled` is true.
    */
   isPressHandledWhenDisabled?: boolean,